-
Notifications
You must be signed in to change notification settings - Fork 422
fix: remove word boundaries from decorator check regex #5534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix: remove word boundaries from decorator check regex #5534
Conversation
Thanks for the contribution! It looks like @yashank-aggarwal_sfemu is an internal user so signing the CLA is not required. However, we need to confirm this. |
What is the motivation for this change? Please also add some test cases to demonstrate the changed behavior. |
@wjhsf The regex condition present was not able to search for the decorator due to ANSI format of the error code given to match, in cases like eg it was able to match the regex due to import statement present and hence threw 1198 error |
When the LWC compiler encounters a babel error, it checks the message for the LWC decorators ( The actual decorator usage that causes the error seems to always be preceded by a color token. However, babel includes a few lines of code as context prior to the erroneous decorator usage. This means that whether we get LWC1198 or LWC1007 is dependent on that context. For example, if the context includes Consider this class, which is not a component: import { eschatology } from 'vermont'
export default class Bananaphone {
@eschatology biggens
} With the proposed change, the above code will fail to compile with "Error: SyntaxError: LWC1198: Decorators like A better fix, here, would be to update the regex to account for the possibility of ANSI escape codes, so that we still only add the LWC context when actually relevant. We should not rely on any specific codes being present, as babel may change the color of their output at any time. |
Agreed to all the points, i am already looking into it and see if some regex can be made to handle that. |
@wjhsf i have updated the regex. Also, looks like updating unit test for this makes no sense as the issue we faced in previous regex was due to ANSI format in error, but in vitest it was a simple string error instead. |
(e as any).message?.includes('Decorators are not enabled.') && | ||
/\b(track|api|wire)\b/.test((e as any).message) // sniff for @track/@api/@wire | ||
// eslint-disable-next-line no-control-regex -- Intentionally matching ANSI escape sequences | ||
/@[\x1b[0-9;m]*(track|api|wire)\b/.test((e as any).message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is brittle. Please just remove the \b
from the original regex and it should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abdulsattar agreed that would also work but there could be a case like following:
import { eschatology } from 'vermont'
export default class Bananaphone {
@eschatology wire
}
then this also throws 1198 instead of 1007 due to wire/api/track being the variable name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or even something that contains any of these words would lead to 1198.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I imagined it would be the case. But, it's an acceptable trade-off. At worst, users are getting a little generic error (which is still related to decorators).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, let me update it then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full sequence, as returned by the ansi-regex npm package, is /(?:\u001B\][\s\S]*?(?:\u0007|\u001B\u005C|\u009C))|[\u001B\u009B][[\]()#;?]*(?:\d{1,4}(?:[;:]\d{0,4})*)?[\dA-PR-TZcf-nq-uy=><~]/g
.
Using that is definitely overkill for our use case. I think we should keep the original check, but relax it if we see the escape character (\x1b
). We don't really care if the escape codes are properly formatted or not. The regex to accomplish this would be /(?:\b|\x1b\S*?)(api|wire|track)\b/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wjhsf the regex you pointed out does filter out some cases like:
import { eschatology } from 'vermont' export default class Bananaphone { @eschatology prewire }
but it would still give 1198 instead of 1007 in cases like:
import { eschatology } from 'vermont' export default class Bananaphone { @eschatology wire }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current regex does not distinguish between @wire prop
and @decorate wire
, so I think it's okay if the updated code also does not distinguish.
Thanks for the contribution! Before we can merge this, we need @yashank676 to sign the Salesforce Inc. Contributor License Agreement. |
@yashank676 please make sure the CLA is signed and add two test cases to |
@wjhsf i have already signed the CLA dont know why its not showing it here.
|
I think these test cases cover the different possible scenarios, but feel free to come up with your own. // Using an LWC decorator -- no comment
import { api } from 'lwc'
export default class NotComponent {
@api prop
}
// Using an LWC decorator -- with comment
import { api } from 'lwc'
// the error message should not include the above import
export default class NotComponent {
@api prop
}
// Using an almost-LWC decorator
const apitrackwire = () => {}
export default class NotComponent {
@apitrackwire prop
}
// Using a non-LWC decorator
const decorator = () => {}
export default class NotComponent {
@decorator prop
} |
should we be adding test cases like:
since ideally they should throw 1007 but since we can't right such stricter regex, it throws 1198 for either regex you mentioned or a simpler one (still confused which one we should use) |
Include whatever test cases you think are relevant, and have them assert the correct error for the regex used. |
/nucleus test |
Details
/\b(track|api|wire)\b/
tests false iftrack
is with ANSI escape codes for color output in terminals e.g.\033[31;1;4mtrack\033[0m
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
LWC1198
error instead ofLWC1007
for Babel decorator error consistently.